Add the with-parent-controls for #1662. #1717

aj-stein-nist · 2023-03-17T21:47:13Z

Committer Notes

Add with-parent-controls to the OSCAL Profile metaschema to match current specification requirements for #1662.

All Submissions:

Have you selected the correct base branch per Contributing guidance?
Have you set "Allow edits and access to secrets by maintainers
"?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Have you squashed any non-relevant commits and commit messages? [instructions]
Do all automated CI/CD checks pass?

By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
~~Have you written new tests for your core changes, as applicable?~~
~~Have you included examples of how to use your new feature(s)?~~
Have you updated all OSCAL website and readme documentation affected by the changes you made? Changes to the OSCAL website can be made in the docs/content directory of your branch.

GaryGapinski

While this isn't the only location, I'll point out that 'true' cast as xs:boolean returns true but 'yes' cast as xs:boolean returns a failure — 'yes' castable as xs:boolean returns false. 'no' castable as xs:boolean returns false.

"yes" and "no" found here and elsewhere were unfortunate choices when XML+XPath should have been considered.

However, the "yes" and "no" values are in accord with the OSCAL Profile Resolution Specification Draft and symmetric with with-child-controls.

aj-stein-nist · 2023-03-20T12:58:29Z

While this isn't the only location, I'll point out that 'true' cast as xs:boolean returns true but 'yes' cast as xs:boolean returns a failure — 'yes' castable as xs:boolean returns false. 'no' castable as xs:boolean returns false.

"yes" and "no" found here and elsewhere were unfortunate choices when XML+XPath should have been considered.

OK, that's a good point that is very much worth calling out. I know "truthiness" and "falsiness" vary a lot in other data formats and programming languages too.

I would like to think about that outside this issue and PR, that requires a more significant analysis and conclusion.

GaryGapinski · 2023-03-20T13:23:10Z

I would like to think about that outside this issue and PR, that requires a more significant analysis and conclusion.

Elsewhere is correct. The concept is essentially "Is it boolean or enumerated", as well as "The attribute values seem to (in English) relate to the element name rather than the attribute".

aj-stein-nist · 2023-03-21T22:08:37Z

Nikita and I have some spec work to do, but we made the necessary prelim changes to the model in the profile metaschemas based on what Wendell proposed.

@wendellpiez we do have some questions about how the spec and the inline metaschema docs document singular or plural and how it reflects the tree. Perhaps we discuss tomorrow?

Add it for insert-controls, but not exclusion or merge, based upon team review and analysis of current profile resolution specification.

…with-parent-controls-attribute-of-include-controls-element

aj-stein-nist · 2023-03-28T22:19:34Z

@wendellpiez I think I am comfortable with you reviewing this now. I tried to simplify and go with a "less is more approach" to specification edits. Also, @GaryGapinski, let me know if you have any questions, comments, or concerns when you review.

GaryGapinski · 2023-03-29T08:30:52Z

While preparing to review this PR and looking at oscal_profile_metaschema.xml I noticed no XML Schema association for the document. Was a bit surprised as that seemed to be a deficit of rigor. Then looked in https://github.com/usnistgov/OSCAL/tree/main/build and found a reference to metaschema @ 973790d, followed that and found (circuitously) toolchains/xslt-M4/validate/metaschema.xsd and used that to validate oscal_profile_metaschema.xml and got a few failed validations.

Realized I should have looked relative to my local clone at the 1662… branch, looked in buiid and found no metaschema reference. Looked in https://github.com/usnistgov/OSCAL/tree/1662-oscal_complete_schemaxsd-v104-lacks-with-parent-controls-attribute-of-include-controls-element/build and found metaschema @ d3d5394 which is accompanied by the somewhat unsettling warning "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." An attempt to force a local clone of https://github.com/usnistgov/metaschema to that commit came to no avail.

Switching to the develop branch of https://github.com/usnistgov/metaschema reveals a schema directory with metaschema.xsd.

When that metaschema.xsd is used to validate oscal_profile_metaschema.xml the validation succeeds. The presumptive success is tarnished a bit by the local clone's 1662… branch lack of a reliable pointer to metaschema and also lacks a local build/metaschema/schema.

All that was to satisfy a desire to look at the metaschema to find out why the old <assembly ref="select-control-by-id" min-occurs="1" max-occurs="unbounded"> min-occurs replaced by the new <define-assembly name="include-controls"> had a min-occurs="1".

A look at xml/schema/oscal_profile_schema.xsd in the local clone's 1662… branch as well as the same branch in the repository shows no change, so I must not be aware of the method by which the metaschema change can be verified.

aj-stein-nist · 2023-03-29T13:11:10Z

@GaryGapinski, ~~nice call outs. I thought our pipeline checks that stuff, but oh well, I guess not! 🤦 I will work on the XSD failures, that was careless of me.~~ OK I had to reread this before some coffee. You can reply here or message in Gitter, is the problem the submodule pointer and showing the derived XML Schema and JSON Schemas to verify.

There is a gap between develop and main that would need pulling up for a release. This PR is pointed at for develop for now, but it seems you figured out that detail based on summary. Is there still some confusion or something I missed besides the <assembly ref="select-control-by-id" max-occurs="unbounded"> versus <assembly ref="select-control-by-id" min-occurs="1" max-occurs="unbounded"> feedback? That's good I need to look into that this morning.

GaryGapinski · 2023-03-29T14:07:58Z

Is there still some confusion or something I missed besides the <assembly ref="select-control-by-id" max-occurs="unbounded"> versus <assembly ref="select-control-by-id" min-occurs="1" max-occurs="unbounded"> feedback?

No: nothing need be done. I just wanted to check the absence of min-occurs found in the previous version by using the metaschema XML Schema. The diff is difficult to review because the metaschema changed the related constructs.

wendellpiez

Happy to help work this further or contribute to due diligence thanks!

src/metaschema/oscal_profile_metaschema.xml

src/specifications/profile-resolution/profile-resolution-specml.xml

wendellpiez · 2023-03-29T15:48:10Z

BTW, if lack of an explicit schema binding is really considered a problem, please consider using the PI-based mechanism described at https://www.w3.org/TR/xml-model/ instead of a more cumbersome and confusing method?

Co-authored-by: Wendell Piez <[email protected]>

…l.xml

aj-stein-nist · 2023-03-30T16:21:05Z

OK, @wendellpiez, I integrated changes given your feedback and clarification in today's pairing session, removing only redundancies in regards to the bullets below with-parent-controls that call out the same info, and shortened some portions of your edits. Let me know when you have a chance if this good enough.

wendellpiez

Caught a couple of small things please look

src/specifications/profile-resolution/profile-resolution-specml.xml

Co-authored-by: Wendell Piez <[email protected]>

…in the profile resolution spec that didn't exist in the model. Instead of updating the model, this PR removes the "with-parent-controls" feature from the profile resolution spec. This developmental feature should be removed for the following reasons. - This feature is not implemented in any of the current XSLT or Java implementations. - This feature is not being requested from a significant segment of the user community. The related issue usnistgov#1662 has support from 1 community member outside the NIST team. - This feature is extremely difficult to implement along with with-child-controls, which works on the opposite axis. - IMHO, profile resolution doesn't need to be made more complicated than it already is.

* Revert changes from #1717 that address a documented feature in the profile resolution spec that didn't exist in the model. Instead of updating the model, this PR removes the "with-parent-controls" feature from the profile resolution spec. This developmental feature should be removed for the following reasons. - This feature is not implemented in any of the current XSLT or Java implementations. - This feature is not being requested from a significant segment of the user community. The related issue #1662 has support from 1 community member outside the NIST team. - This feature is extremely difficult to implement along with with-child-controls, which works on the opposite axis. - IMHO, profile resolution doesn't need to be made more complicated than it already is. * PR review, delete dangling with-parent-controls flag. --------- Co-authored-by: A.J. Stein <[email protected]>

* Revert changes from usnistgov/OSCAL#1717 that address a documented feature in the profile resolution spec that didn't exist in the model. Instead of updating the model, this PR removes the "with-parent-controls" feature from the profile resolution spec. This developmental feature should be removed for the following reasons. - This feature is not implemented in any of the current XSLT or Java implementations. - This feature is not being requested from a significant segment of the user community. The related issue usnistgov/OSCAL#1662 has support from 1 community member outside the NIST team. - This feature is extremely difficult to implement along with with-child-controls, which works on the opposite axis. - IMHO, profile resolution doesn't need to be made more complicated than it already is. * PR review, delete dangling with-parent-controls flag. --------- Co-authored-by: A.J. Stein <[email protected]>

* with-parent-controls for import only for usnistgov#1662 Add it for insert-controls, but not exclusion or merge, based upon team review and analysis of current profile resolution specification. * Clarify spec for usnistgov#1662. * Apply suggestions from code review Co-authored-by: Wendell Piez <[email protected]> * Update src/specifications/profile-resolution/profile-resolution-specml.xml * Apply suggestions from code review Co-authored-by: Wendell Piez <[email protected]> --------- Co-authored-by: Wendell Piez <[email protected]>

* Revert changes from usnistgov#1717 that address a documented feature in the profile resolution spec that didn't exist in the model. Instead of updating the model, this PR removes the "with-parent-controls" feature from the profile resolution spec. This developmental feature should be removed for the following reasons. - This feature is not implemented in any of the current XSLT or Java implementations. - This feature is not being requested from a significant segment of the user community. The related issue usnistgov#1662 has support from 1 community member outside the NIST team. - This feature is extremely difficult to implement along with with-child-controls, which works on the opposite axis. - IMHO, profile resolution doesn't need to be made more complicated than it already is. * PR review, delete dangling with-parent-controls flag. --------- Co-authored-by: A.J. Stein <[email protected]>

aj-stein-nist assigned nikitawootten-nist and aj-stein-nist Mar 17, 2023

aj-stein-nist linked an issue Mar 17, 2023 that may be closed by this pull request

oscal_complete_schema.xsd (v1.0.4) lacks with-parent-controls attribute of include-controls element #1662

Closed

3 tasks

GaryGapinski reviewed Mar 18, 2023

View reviewed changes

aj-stein-nist force-pushed the 1662-oscal_complete_schemaxsd-v104-lacks-with-parent-controls-attribute-of-include-controls-element branch 3 times, most recently from 1987418 to f0d7ff6 Compare March 21, 2023 19:39

aj-stein-nist force-pushed the 1662-oscal_complete_schemaxsd-v104-lacks-with-parent-controls-attribute-of-include-controls-element branch 2 times, most recently from a268d1c to 5151077 Compare March 28, 2023 15:54

with-parent-controls for import only for #1662

55527ef

Add it for insert-controls, but not exclusion or merge, based upon team review and analysis of current profile resolution specification.

aj-stein-nist force-pushed the 1662-oscal_complete_schemaxsd-v104-lacks-with-parent-controls-attribute-of-include-controls-element branch from 5151077 to 55527ef Compare March 28, 2023 22:00

aj-stein-nist added 2 commits March 28, 2023 18:16

Clarify spec for #1662.

3bb55b2

Merge branch 'develop' into 1662-oscal_complete_schemaxsd-v104-lacks-…

28ffd3a

…with-parent-controls-attribute-of-include-controls-element

aj-stein-nist marked this pull request as ready for review March 28, 2023 22:18

aj-stein-nist requested a review from wendellpiez as a code owner March 28, 2023 22:18

wendellpiez reviewed Mar 29, 2023

View reviewed changes

aj-stein-nist and others added 2 commits March 30, 2023 09:48

Apply suggestions from code review

9b8b4db

Co-authored-by: Wendell Piez <[email protected]>

Update src/specifications/profile-resolution/profile-resolution-specm…

478c012

…l.xml

aj-stein-nist requested review from wendellpiez and GaryGapinski March 30, 2023 16:19

wendellpiez previously approved these changes Mar 30, 2023

View reviewed changes

src/specifications/profile-resolution/profile-resolution-specml.xml Outdated Show resolved Hide resolved

src/specifications/profile-resolution/profile-resolution-specml.xml Outdated Show resolved Hide resolved

Apply suggestions from code review

b9d3780

Co-authored-by: Wendell Piez <[email protected]>

aj-stein-nist dismissed wendellpiez’s stale review via b9d3780 March 30, 2023 17:22

aj-stein-nist merged commit 5f16e76 into develop Mar 30, 2023

david-waltermire mentioned this pull request Jun 26, 2023

Remove with-parent-controls developmental feature #1819

Merged

6 tasks

aj-stein-nist added this to the v1.1.0 milestone Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the with-parent-controls for #1662. #1717

Add the with-parent-controls for #1662. #1717

aj-stein-nist commented Mar 17, 2023 •

edited

Loading

GaryGapinski left a comment

aj-stein-nist commented Mar 20, 2023

GaryGapinski commented Mar 20, 2023

aj-stein-nist commented Mar 21, 2023

aj-stein-nist commented Mar 28, 2023

GaryGapinski commented Mar 29, 2023

aj-stein-nist commented Mar 29, 2023 •

edited

Loading

GaryGapinski commented Mar 29, 2023 •

edited

Loading

wendellpiez left a comment

wendellpiez commented Mar 29, 2023

aj-stein-nist commented Mar 30, 2023

wendellpiez left a comment

Add the with-parent-controls for #1662. #1717

Add the with-parent-controls for #1662. #1717

Conversation

aj-stein-nist commented Mar 17, 2023 • edited Loading

Committer Notes

All Submissions:

Changes to Core Features:

GaryGapinski left a comment

Choose a reason for hiding this comment

aj-stein-nist commented Mar 20, 2023

GaryGapinski commented Mar 20, 2023

aj-stein-nist commented Mar 21, 2023

aj-stein-nist commented Mar 28, 2023

GaryGapinski commented Mar 29, 2023

aj-stein-nist commented Mar 29, 2023 • edited Loading

GaryGapinski commented Mar 29, 2023 • edited Loading

wendellpiez left a comment

Choose a reason for hiding this comment

wendellpiez commented Mar 29, 2023

aj-stein-nist commented Mar 30, 2023

wendellpiez left a comment

Choose a reason for hiding this comment

aj-stein-nist commented Mar 17, 2023 •

edited

Loading

aj-stein-nist commented Mar 29, 2023 •

edited

Loading

GaryGapinski commented Mar 29, 2023 •

edited

Loading